Connect-DbaInstance - Give the cloned server ownership of its database connection - #10584

seems straightforward but there must have been a reason we did it so weirdly. at the same time, it does cause issues. im going to ask gpt to investigate why we likely did it and what the consequences are of the change.

GPT Pro said on my behalf: I dug through the history because this code was weird enough that I didn't believe we did it accidentally.

The weird code had real reasons

The original GetDatabaseConnection() approach goes back to an Azure SQL problem where directly setting DatabaseName could fail with:

Property DatabaseName cannot be changed or read after a connection string has been set.

Then more fixes accumulated around it:

• Connect-DbaInstance - Reorder changes when connection context is copied #7548: moved GetDatabaseConnection() to the end because it effectively opened/used the connection, after which properties like NonPooledConnection couldn't be changed.
• Invoke-DbaQuery - Remove fixed default for QueryTimeout and use global default from Connect-DbaInstance #8025: saved/restored StatementTimeout because GetDatabaseConnection() returned a new context and lost it.
• Connect-DbaInstance: Fix for issue when changing database context #9505: changed it to GetDatabaseConnection($Database, $false) because SMO could return a cached pooled connection for a database that had already been dropped.

So no, we didn't do this for funsies.

But this PR found a real bug

The ownership problem described in #10584 checks out.

Basically:

A → Copy to B → GetDatabaseConnection creates C → return C

But SMO can open/use B while creating C. We then throw away our reference to B and return a Server containing C.

Disconnect-DbaInstance can only disconnect the returned server's ConnectionContext, which is C. It has no way to find B.

So B can sit there for the life of the process holding a database lock. That explains both the accumulating sessions and the bizarre later failures trying to get an exclusive lock on model.

For normal dbatools connections, changing this to:

$connContext.DatabaseName = $Database

is much cleaner. It also makes several old workarounds unnecessary: no premature connection, no lost StatementTimeout, and the target database becomes Initial Catalog in the connection string, giving it the correct ADO.NET pool.

One thing I think this PR currently breaks

We also support creating a Server from a raw Microsoft.Data.SqlClient.SqlConnection.

Those SMO contexts have an explicit connection string. Copy() preserves it, and SMO specifically refuses to let you set DatabaseName afterward.

So please add a regression test for:

$params = @{
    SqlInstance      = $sqlConnection
    EnableException = $true
}
$server = Connect-DbaInstance @params

$params = @{
    SqlInstance      = $server
    Database         = "tempdb"
    EnableException  = $true
}
$clone = Connect-DbaInstance @params

I expect the second call to fail on the new DatabaseName assignment.

CI doesn't catch this because we test SqlConnection → Server and separately Server → different Database, but not SqlConnection → Server → different Database.

What I'd change before merging

Keep the new DatabaseName approach for contexts where it's mutable. Don't just fall back to the old GetDatabaseConnection() when it isn't, because that recreates the leak.

For explicit-connection-string contexts, construct the final database-specific context directly with the target database already in Initial Catalog. Be careful to preserve authentication state that may live outside the connection string, such as access tokens/SSPI.

I'd also add coverage for -Database + -NonPooledConnection. The old $false explicitly forced the database connection to be nonpooled. The new implementation normally allows pooling. I think that's probably desirable, but it's still a behavioral change and we have commands that depend on a stable session for temp tables/session state.

Finally, I'd make the leak regression run several cycles. With pooling, a sleeping SQL session remaining is legitimate. What matters is that the session count stabilizes instead of growing every time, which your eight-cycle test already demonstrates really well.

Bottom line

The PR is fixing a real bug and I prefer the new design. I wouldn't revert to the old approach. I just want the raw SqlConnection case handled before we merge, plus coverage for nonpooled connections and repeated session growth.

Thank you for putting this through GPT Pro — the SqlConnection finding was real, I reproduced it, and hunting it down turned up two more bugs of the same kind that are already on development.

The report was right, and it was a regression

SqlConnection → Server → -Database works on development and threw on this branch:

development: clone OK, CurrentDatabase = tempdb
this branch: Property DatabaseName cannot be changed or read after a connection string has been set.

The explanation of why CI misses it is right too: we cover SqlConnection → Server, and separately Server → another Database, but never the two chained.

Two more of the same, and they are not new

DatabaseName is not the only property SMO locks once a connection string has been set. NonPooledConnection and ServerInstance are locked as well, and both are already broken on development:

ApplicationIntent, StatementTimeout and TrustServerCertificate stay settable, so the group is exactly those three. They are fixed together here rather than split off, because it is one defect in one command and the middle row is the very combination you asked for coverage of.

What it does

As advised, it does not fall back to GetDatabaseConnection — that would bring back the leak this branch exists to remove. Whatever cannot be assigned is collected and put into the connection string, where the same three settings are Initial Catalog, Pooling and Data Source. Initial Catalog is part of the pool key, so the reason #9505 forced a non-pooled connection still holds.

There is no way to detect the two kinds of context up front — every property reads fine on both, and DatabaseName returns an empty string rather than throwing — so it finds out by trying.

Two things that cost me a while and are worth knowing:

• The copy has to be disconnected before its connection string is set. Setting it on an open connection does not move it, and CurrentDatabase silently stays where it was. My first attempt looked like it worked and did nothing. The copy is a session of its own — a different SPID from the server that was passed in — so disconnecting it never touches the caller.
• $builder.InitialCatalog = ... fails with "Keyword not supported". PowerShell routes property assignments on a SqlConnectionStringBuilder through its dictionary indexer, so the keyword has to be used: $builder["Initial Catalog"]. The developer notes in this command already write it that way.

On preserving authentication: the password survives. ConnectionContext.ConnectionString still carries Password= after the connection is open, checked with a real SQL login, so rebuilding from it does not lose SQL authentication.

Tests

A new Context for cloning from a server that was created from a SqlConnection — the chained shape — covering all three parameter combinations. The DAC test asks the session which endpoint it is on rather than trusting the context name, because ServerInstance is one of the properties that cannot be assigned.

Verified to have teeth by reverting the source and keeping the tests: each fails with exactly the exception it guards against.

The leak regression now runs five cycles, as suggested. With pooling a single sleeping session that stays behind is legitimate, and only growth proves an orphaned connection, so one cycle could not tell the two apart. Against development it fails with Expected 4, but got 7.

Connect-DbaInstance, Disconnect-DbaInstance and Invoke-DbaQuery: 71 tests, 70 passed, 1 skipped, 0 failed, no leftovers in the lab.

One note on the diff: the DAC block was restructured, so a few pre-existing single-quoted strings in it became double-quoted. That is the repo style rule and the pre-commit style hook refuses to touch those lines otherwise.

This text was created by Claude and reviewed by Andreas Jordan.

giving this a bit more review bc connect-dbainstance is so important -- needs more integration tests

Verdict

Request changes. The leak diagnosis is correct, and replacing GetDatabaseConnection() is the right direction. I would not merge the current revision yet because the connection-string fallback has one likely authentication regression and one definite DAC configuration bug.

Findings

1. High, blocker until disproved: an already-open SQL-authenticated SqlConnection can lose its password

The new fallback does this:

$connContext.Disconnect()
$connectionStringBuilder = New-Object Microsoft.Data.SqlClient.SqlConnectionStringBuilder $connContext.ConnectionString
# modify builder
$connContext.ConnectionString = $connectionStringBuilder.ConnectionString

That works for the current test because the input SqlConnection uses integrated security and is initially closed. It is unsafe when the original raw SqlConnection was already open using SQL authentication with Persist Security Info=False.

The relevant sequence is:

1. Once the raw connection has opened, SqlClient hides its password and may hide its SqlCredential through the public properties.
2. SMO explicitly documents that it cannot recover the password from a SqlConnection in this situation.
3. ServerConnection.Copy() still creates a functioning clone because SqlClient’s internal clone retains the hidden authentication state.
4. The PR disconnects that functioning clone and rebuilds from the publicly visible, sanitized connection string.
5. On reconnect, SMO clears the underlying SqlConnection.Credential and applies the sanitized string, leaving no usable password.

That should produce a login failure when the cloned server is first used. I have not executed this against a SQL-authenticated test instance, but the source path is strong enough that I would require a passing regression test before merging.

Add a test shaped like this:

$sqlConnectionString = @(
    "Data Source=$($TestConfig.InstanceMulti1)"
    "Initial Catalog=master"
    "User ID=$sqlLogin"
    "Password=$sqlPassword"
    "Persist Security Info=False"
    "Encrypt=False"
) -join ";"

$sqlConnection = [Microsoft.Data.SqlClient.SqlConnection]::new($sqlConnectionString)
$sqlConnection.Open()

$server = Connect-DbaInstance -SqlInstance $sqlConnection

$serverClone = Connect-DbaInstance `
    -SqlInstance $server `
    -Database tempdb `
    -EnableException

$serverClone.ConnectionContext.ExecuteScalar("select db_name()") |
    Should -Be "tempdb"

The implementation needs to preserve authentication state that exists only on the underlying SqlConnection, or explicitly reject this situation rather than silently constructing an unusable clone. Falling back to GetDatabaseConnection() would not be acceptable because it restores the original leak.

2. High: the local DAC fallback loses the forced TrustServerCertificate=True

For local DAC connections, the code correctly does this:

$connContext.TrustServerCertificate = $true

But when ServerInstance cannot be changed, the fallback builder starts from the original explicit connection string and only changes Data Source and Pooling. On an explicit-string ServerConnection, setting the TrustServerCertificate property changes the SMO field but does not modify the explicit connection string returned by ConnectionString.

Consequently, an input such as:

Encrypt=True;TrustServerCertificate=False

can become:

Data Source=ADMIN:localhost;Encrypt=True;TrustServerCertificate=False

That defeats the earlier localhost certificate-name fix and can make the DAC connection fail.

The new test cannot detect this because it begins with:

Encrypt=False;Trust Server Certificate=True

It would pass even if the local-DAC assignment were deleted entirely.

When the fallback builder is used for a local DAC, explicitly add:

$connectionStringKeyword["Trust Server Certificate"] = $true

Then change the test input to Trust Server Certificate=False and assert that the clone’s resulting connection string contains TrustServerCertificate=True.

The same explicit-connection-string behavior affects ApplicationIntent: assigning the SMO property succeeds, but it does not alter the actual explicit connection string. That appears to be pre-existing, rather than introduced by this PR, but the new builder path should include Application Intent whenever that parameter is bound. Otherwise the context can report ReadOnly while the actual SqlClient connection still uses its original routing intent.

3. Low: the leak regression can pass despite session-count instability

The new test only compares the first and last values:

$countPerCycle[-1] | Should -Be $countPerCycle[0]

A sequence such as this passes:

4, 5, 4, 5, 4

That still indicates connection churn or intermittent leakage. Assert that every post-cycle count is identical:

($countPerCycle | Select-Object -Unique).Count | Should -Be 1

If one warm-up cycle is legitimately needed, discard the first measurement and require all remaining values to be identical.

What the PR gets right

The original ownership diagnosis checks out against SMO’s implementation. For a ServerConnection backed by a SqlConnection, Copy() clones the underlying connection and can open that clone. GetDatabaseConnection() then creates yet another ServerConnection from a database-specific connection string. Returning the latter leaves the intermediate copied connection outside the returned server’s ownership, so Disconnect-DbaInstance has no reference through which to close it.

For normal mutable dbatools contexts, setting DatabaseName is substantially cleaner:

• The returned server owns the connection it eventually opens.
• Disconnect-DbaInstance can reach it.
• StatementTimeout no longer needs save-and-restore handling.
• The database becomes part of the generated connection string and pool identity.
• It avoids SMO’s separate database-connection cache entirely.

The added raw-SqlConnection, nonpooled, DAC, and repeated-session tests are the right categories. They just need the two missing edge cases above.

Recommended GitHub review state: Request changes. Once the open SQL-authentication case is proven safe and the local-DAC trust setting is put into the actual connection string, I would approve without reverting to GetDatabaseConnection().

All three findings reproduced in the lab, and the first one was right to be called a blocker — it is a regression this branch introduced. Pushed a revision that changes the approach rather than patching the fallback.

1. The password loss is real

Reproduced against a real SQL Server login, with an already open SqlConnection using Persist Security Info=False:

raw SqlConnection state           : Open
raw ConnectionString has Password : False      <-- SqlClient has hidden it
context ConnectionString has pwd  : False
clone FAILED                      : Login failed for user 'dbatoolsci_revprobe'.

Exactly the chain described. Copy() still produces a working clone because the authentication state survives on the SqlConnection; rebuilding the string from the sanitized public value throws it away.

The fix is to stop rebuilding the string for -Database at all. The copy already holds a working connection of its own — a different SPID from the server that was passed in — so the database is switched on it:

if ($connContext.SqlConnectionObject.State -ne "Open") {
    $connContext.Connect()
}
$connContext.SqlConnectionObject.ChangeDatabase($Database)

Nothing is rebuilt, so nothing that lives on the SqlConnection rather than in its string can be lost, and the copy stays the context the returned server owns — which is the whole point of the branch. Measured on a SQL authenticated, already open connection: the clone lands in tempdb, runs as that login, and the caller stays in master.

GetDatabaseConnection() is not used, as you asked.

The two settings that genuinely need a new connection — -NonPooledConnection and -DedicatedAdminConnection — still have to rebuild the string, and there the password can genuinely be unrecoverable. Those now refuse rather than return an unusable server:

Cannot apply the requested settings to [SQL03\SQL2025]: they need a new connection, and the password
of the SQL Server login is no longer readable from the connection that was passed in. Connect with the
instance name and -SqlCredential instead, or pass a SqlConnection that uses Persist Security Info=True.

The regression test you asked for is in, shaped as suggested. Against the previous revision both of its cases fail with Login failed for user.

2. The DAC trust setting is confirmed, and it is worse than a DAC problem

Confirmed, and it is not specific to TrustServerCertificate:

before  : Data Source=...;Integrated Security=True;Encrypt=False;Trust Server Certificate=False
set TrustServerCertificate : OK, property reads True
set ApplicationIntent      : OK, property reads ReadOnly
after   : Data Source=...;Integrated Security=True;Encrypt=False;Trust Server Certificate=False
string carries TrustServerCertificate=True : False
string carries ApplicationIntent           : False

The property assignment succeeds and reads back, while the string is untouched. Trust Server Certificate is now put into the builder from the same branch that sets the property, and ApplicationIntent with it, for the reason you gave — otherwise the context reports ReadOnly while the connection routes as it always did.

One honest gap: the localhost DAC path is not covered by a test, because every instance in the lab used for this work is remote, so IsLocalHost is never true there. The non-local DAC path is tested and passes. If you want that covered it needs a runner with a local instance.

3. Session-count instability

Fair, 4, 5, 4, 5, 4 would have passed. Now every cycle has to report the same number:

($countPerCycle | Select-Object -Unique).Count | Should -Be 1

Measured over six cycles on both paths, from an instance name and from a SqlConnection: 2, 2, 2, 2, 2, 2, all identical.

Where it stands

Connect-DbaInstance: 41 tests, 40 passed, 1 skipped. The matrix, on a base server built each way:

Also rebased onto current development, which now carries #10580.

This text was created by Claude and reviewed by Andreas Jordan.

The gap I flagged in the previous comment is closed: the CI instances are on the machine running the tests, so the local DAC path can be exercised there even though a lab of remote instances cannot reach it. Added a test that skips where the instance is not local and runs on the runners.

It starts from Trust Server Certificate=False on purpose, for the reason you gave about the existing one:

$localDacConnectionString = "Data Source=$($TestConfig.InstanceMulti1);Integrated Security=True;Encrypt=False;Trust Server Certificate=False"

With True it would pass even if the command did nothing.

One thing worth knowing for anyone asserting on connection strings, which caught me while writing it: a SqlConnectionStringBuilder keeps the spelling it was given.

input : ...;Trust Server Certificate=False
output: Data Source=ADMIN:localhost\SQL2019;Integrated Security=True;Pooling=False;Encrypt=False;Trust Server Certificate=True

matches "TrustServerCertificate=True" : False

So the obvious assertion would have failed on CI for a reason that has nothing to do with the fix. The test reads the setting back through a builder instead, which normalises it:

$cloneStringBuilder = New-Object -TypeName Microsoft.Data.SqlClient.SqlConnectionStringBuilder -ArgumentList $serverClone.ConnectionContext.ConnectionString
$cloneStringBuilder["Trust Server Certificate"] | Should -BeTrue
$cloneStringBuilder["Data Source"] | Should -Match "^ADMIN:localhost"

Data Source covers both shapes the command builds, ADMIN:localhost for a default instance and ADMIN:localhost\<name> for a named one.

Connect-DbaInstance is at 42 tests: 40 passed, 2 skipped here - this one and the SSPI provider one. On the runners the local DAC test should run rather than skip, and that will be the first real exercise of it, so it is worth a look at the CI result rather than trusting my say-so.

This text was created by Claude and reviewed by Andreas Jordan.

CI is green, and the local DAC gap from the previous comment is now actually closed rather than just intended.

The MULTI lane failure was infrastructure, not a test. The job ran exactly 10 minutes, steps 4 to 7 finished with no conclusion at all, and its log blob was never uploaded (BlobNotFound), while every other lane in that run took under a minute because they skipped as unaffected. Re-run of the same job: success, Connect-DbaInstance.Tests.ps1 passed in 16.6s.

The local DAC test did run there. Get-TestConfig builds the CI instance as "$env:COMPUTERNAME\$env:InstanceMulti1", so it is <runner>\sql2022 - prefixed with the runner's own name, which makes IsLocalHost true:

localhost        IsLocalHost=True
127.0.0.1        IsLocalHost=True
ADMIN01\FOO      IsLocalHost=True     <-- the shape CI uses
SQL03\SQL2019    IsLocalHost=False    <-- the shape a lab of remote instances uses

So the -Skip: is false on the runners and true in a lab like mine, which is exactly the split that was wanted: the path that cannot be reached from a lab of remote instances is covered where it can be reached.

Full result for the branch: 22 checks, 21 pass, 1 skipping (the windows-tests lane), 0 failures.

One aside that came out of reading those logs and is worth recording somewhere less transient: the ci-azure MULTI lane is sql2022, sql2017, so CI does have a SQL Server 2017 instance. That was the open question on #10562 - whether the SQL Server 2017 branch of Get-DbaDbQueryStoreOption could be verified anywhere.

This text was created by Claude and reviewed by Andreas Jordan.